Fix certifier test permissions - #13473
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the certifier gold tests to avoid permission failures when the CI harness runs as root but the ATS process runs unprivileged, ensuring ATS can update the copied CA serial file and certificate store across all certifier scenarios.
Changes:
- Creates explicit
store_pathandserial_pathvariables for each certifier scenario setup. - Adjusts permissions on the copied
ca-serial.txtand thestore/directory to allow the unprivileged ATS process to write.
Suppressed comments (3)
tests/gold_tests/pluginTest/certifier/certifier.test.py:135
- Using Setup.RunCommand with an unquoted shell command (and
&&) is brittle: paths with spaces/shell metacharacters will break, and this needlessly invokes a shell. Prefer adjusting permissions via Python (os.chmod) in-process.
Setup.RunCommand(f'chmod a+rw {serial_path} && chmod a+rwx {store_path}')
tests/gold_tests/pluginTest/certifier/certifier.test.py:195
- Using Setup.RunCommand with an unquoted shell command (and
&&) is brittle: paths with spaces/shell metacharacters will break, and this needlessly invokes a shell. Prefer adjusting permissions via Python (os.chmod) in-process.
Setup.RunCommand(f'chmod a+rw {serial_path} && chmod a+rwx {store_path}')
tests/gold_tests/pluginTest/certifier/certifier.test.py:261
- Using Setup.RunCommand with an unquoted shell command (and
&&) is brittle: paths with spaces/shell metacharacters will break, and this needlessly invokes a shell. Prefer adjusting permissions via Python (os.chmod) in-process.
Setup.RunCommand(f'chmod a+rw {serial_path} && chmod a+rwx {store_path}')
There was a problem hiding this comment.
I'd much prefer not to use shell commands for this and just use os.chmod as copilot says. If I understand it correctly, there is a Setup function to run a function at test time that could be used. Minor thing, but avoiding shell commands is always a win. Could also reduce duplication with a function.
Certifier tests fail in root-run CI because ATS cannot update the copied serial file or certificate store. Local owner-run tests mask the problem. This problem is addressed in this patch by giving the unprivileged ATS process the required access to the serial file and certificate store in each certifier scenario.
524b98d to
1280027
Compare
Yeah, thanks for pushing back on this. I implemented this now as a Setup lambda function. Can you please re-review? |
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
Lovely. Thank you for the cleanup!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/gold_tests/pluginTest/certifier/certifier.test.py:36
- Granting world-writable permissions (0666/0777) is overly permissive and can be rejected by hardened CI environments (and can mask real permission/ownership issues). Prefer least-privilege by changing ownership to the ATS runtime user/group (or the test runner’s effective user/group) and then using tighter modes (e.g., 0660 for the serial file and 0770 for the store directory). If the harness provides a
Setup.Chown/Setup.Chgrputility, use that instead of making files world-writable.
os.chmod(serial_path, 0o666)
os.chmod(store_path, 0o777)
tests/gold_tests/pluginTest/certifier/certifier.test.py:36
- Only the top-level
storedirectory permissions are updated. IfSetup.Copy()brings in existing files/subdirectories understore/with restrictive permissions, ATS may still be unable to overwrite/update those entries, leaving the root-run CI failure unresolved for some scenarios. Consider recursively applying writable permissions to existing contents ofstore/(directories and files), or ensure the copied store is empty and created fresh with the intended ownership/permissions.
def set_permissions() -> None:
os.chmod(serial_path, 0o666)
os.chmod(store_path, 0o777)
Certifier tests fail in root-run CI because ATS cannot update the
copied serial file or certificate store. Local owner-run tests mask the
problem.
This problem is addressed in this patch by giving the unprivileged ATS
process the required access to the serial file and certificate store in
each certifier scenario.